Skip to content

Add BackendReader trait#986

Merged
sergerad merged 8 commits into
nextfrom
sergerad-forest-reader-trait
May 14, 2026
Merged

Add BackendReader trait#986
sergerad merged 8 commits into
nextfrom
sergerad-forest-reader-trait

Conversation

@sergerad
Copy link
Copy Markdown
Contributor

@sergerad sergerad commented May 1, 2026 •
edited
Loading

Context

In #967 we created reader traits used by rocks db structs including a new snapshot reader struct.

We need to do the same for the large forest SMT traits and PersistentBackend.

Changes

  • Extracts a BackendReader trait from Backend, providing the read-only query interface for the SMT forest storage backend (open, get, get_leaf, version, lineages, trees, entries, entry_count).
  • Backend becomes a supertrait of BackendReader, extending it with write operations and an associated Reader type with a reader() method that returns a point-in-time snapshot.
  • Adds InMemoryBackendSnapshot — a read-only, snapshot of the in-memory backend — implementing only BackendReader.
  • Adds PersistentBackendReader — a read-only view of the persistent backend backed by a RocksDB snapshot, providing a consistent read view isolated from concurrent writes (Similar to RocksDbSnapshotStorage).

Base automatically changed from sergerad-largesmt-reader-trait to next May 1, 2026 02:36
@sergerad sergerad marked this pull request as ready for review May 4, 2026 01:41
@huitseeker
Copy link
Copy Markdown
Collaborator

huitseeker commented May 4, 2026

@sergerad I think this needs a rebase on the squash-merged #967

@sergerad
Copy link
Copy Markdown
Contributor Author

sergerad commented May 4, 2026

@sergerad I think this needs a rebase on the squash-merged #967

Why do you say that? I don't see a problem with the diffs after the base was automatically changed to next. Am I missing something?

@huitseeker
Copy link
Copy Markdown
Collaborator

huitseeker commented May 5, 2026

@sergerad the history of this PR is clean from the diff PoV but not from the commit sequence PoV, as it includes each individual commit of #967 as opposed to a7f748e. Cleaning this up requires a rebase on the latest origin/next.

@sergerad
Copy link
Copy Markdown
Contributor Author

sergerad commented May 5, 2026

@sergerad the history of this PR is clean from the diff PoV but not from the commit sequence PoV, as it includes each individual commit of #967 as opposed to a7f748e. Cleaning this up requires a rebase on the latest origin/next.

Makes sense. But is that not moot given this branch's commits will be squashed before merge?

@huitseeker
Copy link
Copy Markdown
Collaborator

huitseeker commented May 5, 2026

@sergerad FWIW I find helping reviewers navigate PRs faster is one of the most impactful accelerants to shipping overall.

@sergerad sergerad force-pushed the sergerad-forest-reader-trait branch from 5b83c87 to 5731053 Compare May 5, 2026 20:23
@sergerad
Copy link
Copy Markdown
Contributor Author

sergerad commented May 5, 2026

@huitseeker I have rebased now. Note this branch's commits have been squashed as I avoided manual merging / cherry-picking to perform the rebase. Hope that is OK.

@huitseeker
Copy link
Copy Markdown
Collaborator

huitseeker commented May 6, 2026

@sergerad quite fine! Note a good way to perform this sort of cleanup w/o deleting commit history is to do an rebase (possibly an interactive one to visually check the results) skipping the empty (metadata-only) commits: git rebase ... --empty=drop

huitseeker
huitseeker requested changes May 6, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main API design question is whether LargeSmtForest should expose a reader() method analogous to LargeSmt::reader(). Without it, callers that already hold a forest cannot obtain a read-only forest snapshot because the backend field is private.

Comment thread miden-crypto/src/merkle/smt/large_forest/backend/persistent/tests.rs
@sergerad
Copy link
Copy Markdown
Contributor Author

sergerad commented May 7, 2026

The main API design question is whether LargeSmtForest should expose a reader() method analogous to LargeSmt::reader(). Without it, callers that already hold a forest cannot obtain a read-only forest snapshot because the backend field is private.

Have added LargeSmtForest::reader()

@sergerad sergerad requested a review from huitseeker May 7, 2026 00:47
huitseeker
huitseeker approved these changes May 7, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM!

bobbinth
bobbinth approved these changes May 13, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! Not a super in-depth review, but I left a few minor comments inline.

Comment thread CHANGELOG.md Outdated
Comment thread miden-crypto/src/merkle/smt/large_forest/backend/mod.rs
/// Methods may place additional constraints on which errors are used to signal certain failures.
/// Such failures should not lead to data corruption of any persistent data.
pub trait Backend
pub trait BackendReader
Copy link
Copy Markdown
Contributor

@bobbinth bobbinth May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (feel free to ignore): Would ReadOnlyBackend be a better name?

Copy link
Copy Markdown
Contributor Author

@sergerad sergerad May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but would need to update SmtStorageReader and SparseMerkleTreeReader also for consistency.

Comment thread miden-crypto/src/merkle/smt/large_forest/backend/persistent/mod.rs
@sergerad sergerad enabled auto-merge (squash) May 14, 2026 00:43
@sergerad sergerad merged commit 45d5846 into next May 14, 2026
27 of 29 checks passed
@sergerad sergerad deleted the sergerad-forest-reader-trait branch May 14, 2026 00:55
@huitseeker huitseeker mentioned this pull request May 14, 2026
Merged
@huitseeker
Copy link
Copy Markdown
Collaborator

huitseeker commented May 14, 2026 •
edited
Loading

@sergerad Merged on a red CI status, fyi

@sergerad
Copy link
Copy Markdown
Contributor Author

sergerad commented May 14, 2026

@huitseeker thanks for letting me know. Wasn't expecting automerge to go ahead on red.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@huitseeker huitseeker huitseeker approved these changes

@bobbinth bobbinth bobbinth approved these changes

@iamrecursion iamrecursion Awaiting requested review from iamrecursion

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sergerad @huitseeker @bobbinth